-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate Index Management to the new ES JS client #105863
Migrate Index Management to the new ES JS client #105863
Conversation
2138084
to
c2d901f
Compare
4db07da
to
cb25a9b
Compare
42424de
to
0d53fc5
Compare
* Destructure index API request bodies consistently. * Update tests. * Migrate enrichers.
- Migrate create and update index template routes to handleEsError.
* Fix simulate endpoint and move elasticsearch/issues/59152 workaround to server. * Remove commented-out code and unnecessary types from fetch_indices. * Convert to IScopedClusterClient in a few places. * Delete wrapEsError helpers. * Remove unnecessary calls to encodeURIComponent on the server.
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
0d53fc5
to
fead8c1
Compare
} = template; | ||
|
||
// Verify the template exists (ES will throw 404 if not) | ||
const { body: templateExists } = await doesTemplateExist({ name, client, isLegacy }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the logic inside the try/catch
so we could catch any ES errors from the helpers.
@@ -281,8 +281,19 @@ export default function ({ getService }: FtrProviderContext) { | |||
expect(body).to.eql({ | |||
statusCode: 404, | |||
error: 'Not Found', | |||
message: | |||
'[resource_not_found_exception] component template matching [component_does_not_exist] not found', | |||
message: 'component template matching [component_does_not_exist] not found', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The errors we get now are more consistent with what we get from ES, which I think is a good thing for making our error-handling UX more consistent (see #84801).
includeDefaults: true, | ||
const { | ||
body: { persistent, transient, defaults }, | ||
} = await client.asCurrentUser.cluster.getSettings({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test this -- could someone look into setting these settings manually and verifying there are no errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented via #43901.
Looks like it is working as expected for both composable and legacy index templates.
@@ -193,8 +193,8 @@ export default function ({ getService }) { | |||
}); | |||
|
|||
it('should parse the ES error and return the cause', async () => { | |||
const templateName = `template-${getRandomString()}`; | |||
const payload = getTemplatePayload(templateName, [getRandomString()]); | |||
const templateName = `template-create-parse-es-error}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found these hardcoded strings very handy when I was debugging these tests by looking at the functional test runner logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this @cjcenizal! Great to see all of our apps migrated 🎉 . I left a few comments/questions in the code. Let me know what you think.
(@alisonelizabeth) Go through the indices, index templates, and component templates tabs and verify that the basic CRUD operations work. See "Special test cases" below for further testing.
(@alisonelizabeth) Examine each tab in the index details flyout and verify there aren't any errors. Edit an index's settings.
👍 Tested these items and everything worked as expected. I'll defer to @sabarasaba on the other items.
Also, now that we've migrated all of our apps, can we safely delete the is_es_error
util from es_ui_shared
? (Perhaps as a follow-up item.)
|
||
## Index templates tab | ||
|
||
### Quick steps for texting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Quick steps for texting | |
### Quick steps for testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add a section on how to test cloud-managed templates?
See instructions on #43901.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch, done.
documents: hit['docs.count'], | ||
size: hit['store.size'], | ||
isFrozen: hit.sth === 'true', // sth value coming back as a string from ES | ||
aliases: aliases.length ? aliases : 'none', | ||
// @ts-expect-error Property 'index' does not exist on type 'IndicesIndexSettings | IndicesIndexStatePrefixedSettings'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe any ts errors due to lack of coverage/incorrect types in the es js client should be commented as // @ts-expect-error @elastic/elasticsearch ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's incorrect typing - according to @elastic/elasticsearch
typings it should be refactored to hidden: index.settings.hidden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested this and verified that the original path (index.settings.index.hidden
) is correct. I've updated all the annotations to be // @ts-expect-error @elastic/elasticsearch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cjcenizal if you are sure there is a problem you should report it to the client team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This is listed as a follow-up item in the PR description.
includeDefaults: true, | ||
const { | ||
body: { persistent, transient, defaults }, | ||
} = await client.asCurrentUser.cluster.getSettings({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented via #43901.
Looks like it is working as expected for both composable and legacy index templates.
const getDataStreamsStats = (client: ElasticsearchClient, name = '*') => { | ||
return client.transport.request({ | ||
const getDataStreamsStats = (client: IScopedClusterClient, name = '*') => { | ||
return client.asCurrentUser.transport.request({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this comment apply here as well, or can we use client.indices.dataStreamsStats
?
// TODO update when elasticsearch client has update requestParams...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed.
@@ -68,9 +68,9 @@ const enhanceDataStreams = ({ | |||
}); | |||
}; | |||
|
|||
const getDataStreams = (client: ElasticsearchClient, name = '*') => { | |||
const getDataStreams = (client: IScopedClusterClient, name = '*') => { | |||
// TODO update when elasticsearch client has update requestParams for 'indices.getDataStream' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to open an issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method is already available in the new client https://github.com/elastic/kibana/blob/b84b51cc02e78ba76c0de289aa379b0f7144263d/x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts#L498
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fixed.
|
||
return response.ok({ body: responseBody }); | ||
} catch (error) { | ||
return handleEsError({ error, response }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the handleEsError
util provide the same functionality as before? I see previously it looks like we were parsing the error and passing it to body.attributes
.
const error = lib.parseEsError(e.response);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
return response.ok({ body }); | ||
} catch (error) { | ||
if (isEsError(error)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the handleEsError
util here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might be the only case where we're still using isEsError
, so if we're able to remove it here, we should also be able to remove it from shared_imports
and the api routes setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! Done.
throw e; | ||
return response.ok({ body: templatePreview }); | ||
} catch (error) { | ||
return handleEsError({ error, response }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here about the previous behavior of parsing the ES error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one @cjcenizal! I tested all the following items items and I didn't find any regressions:
(@sabarasaba) Test the CCR, ILM, and Rollup enrichers by following the testing steps in each of those plugins' READMEs.
(@sabarasaba) Try out the different index-level actions in Index Management: freeze, unfreeze, open, close, and so on. Perform these actions on both individual indices and on bulk indices. See "Special test cases" below for further testing.
(@sabarasaba) Follow the instructions in the Index Management README to create a data stream. Verify that it appears in the data streams tab and that toggling "Include stats" surfaces additional stats in the table. See "Special test cases" below for further testing.
+special cases
* With steps for testing Cloud-managed index templates * With steps for testing indices and data streams that contain special characters
* Remove unused isEsError and parseEsError dependencies.
b84b51c
to
88a2e09
Compare
Thanks for the reviews @sabarasaba and @alisonelizabeth! Alison, I believe I've addressed your comments. Could you please take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my feedback @cjcenizal! Did not retest, but reviewed the changes and everything LGTM.
0b4f32c
to
0ed2d64
Compare
Thanks @cjcenizal! Latest changes lgtm also 🥳 |
…ic#105863) * Destructure index API request bodies consistently. * Remove unnecessary calls to encodeURIComponent on the server. * Migrate routes to handleEsError. Delete wrapEsError helpers. Remove unused isEsError and parseEsError dependencies. Remove isEsError from es_ui_shared. * Update tests and migrate API integration tests. * Clarify test details in CCR README. Update Index Management README with steps for testing Cloud-managed index templates and steps for testing indices and data streams that contain special characters. # Conflicts: # x-pack/plugins/index_management/server/routes/api/templates/register_create_route.ts # x-pack/plugins/index_management/server/routes/api/templates/register_get_routes.ts # x-pack/plugins/index_management/server/routes/api/templates/register_update_route.ts
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsAPI count
API count missing comments
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
…105863) (#107431) * Migrate Index Management and enrichers to the new ES JS client (#105863) * Destructure index API request bodies consistently. * Remove unnecessary calls to encodeURIComponent on the server. * Migrate routes to handleEsError. Delete wrapEsError helpers. Remove unused isEsError and parseEsError dependencies. Remove isEsError from es_ui_shared. * Update tests and migrate API integration tests. * Clarify test details in CCR README. Update Index Management README with steps for testing Cloud-managed index templates and steps for testing indices and data streams that contain special characters. # Conflicts: # x-pack/plugins/index_management/server/routes/api/templates/register_create_route.ts # x-pack/plugins/index_management/server/routes/api/templates/register_get_routes.ts # x-pack/plugins/index_management/server/routes/api/templates/register_update_route.ts * Fix include_type_name type.
…ic#105863) * Destructure index API request bodies consistently. * Remove unnecessary calls to encodeURIComponent on the server. * Migrate routes to handleEsError. Delete wrapEsError helpers. Remove unused isEsError and parseEsError dependencies. Remove isEsError from es_ui_shared. * Update tests and migrate API integration tests. * Clarify test details in CCR README. Update Index Management README with steps for testing Cloud-managed index templates and steps for testing indices and data streams that contain special characters.
Closes #73973
This PR migrates Index Management to the new ES JS client. Other changes:
handleEsError
pattern.Note that I added numerous
@ts-expect-error
comments to silence TS complaints. I expect that we'll address these in subsequent PRs. I also discovered UX flaws and test coverage gaps, which I noted under my "Notes" section below. We'll also address these in subsequent PRs. All of these are noted under the "Follow-up items" section below.Things to test
I've made suggestions about how to break up this testing work, but feel free to disregard this if you'd prefer to test in a different way.
Special test cases
Indices with special characters
We added support for special characters in index names like
%{[@metadata][beat]}-%{[@metadata][version]}-2020.08.23
with #76584. You can create this index in Console like this:Test that all actions can be performed on this index.
Data streams with special characters
Similar to the above, data streams also support special characters in their names and we need to test that we can fetch them, both with and without stats, and also delete them. You can create this kind of data stream in Console like this:
Legacy index templates
Follow the Index Management README instructions to get legacy index templates to appear in the UI.
Follow-up items
I'll create individual issues to track these items.
@ts-expect-error
comments where possible and file issues for missing/incorrect types inelasticsearch-specification
Notes
To-do
handleEsError
SectionError
TODO update when elasticsearch client has update requestParams for 'indices.getDataStream'
-- ignoring for now to minimize scopeRoute audit
Component templates
Data streams
Indices
Notes: Tested listing, freezing, and unfreezing with indices with special characters, per #76584.
maxNumSegments
optionMapping
Notes: Tested with special characters, per #76584.
Settings
Notes: Tested with special characters, per #76584.
Stats
Notes: Tested with special characters, per #76584.
Index templates
Error-handling
Index Management privileges
When testing with a user with the following privileges, Index Management is accessible, but the Indices and Data Streams tabs report the privileges error.
Reducing it further shows error for index and component templates too:
Note: Component templates has a specific privileges check for
manage_index_templates
and renders a custom error message.Deleting index templates
If you delete an index template that's in use by a data stream, you'll get back lots of error details but they aren't surfaced in the UI. Ideally you should be able to click a button in the error toast to get more info.
Deleting component templates
Similarly, if you delete component templates that no longer exist, you'll get back lots of error details but they aren't surfaced in the UI. Ideally you should be able to click a button in the error toast to get more info.